Skip to content

Conversation

@rpanackal
Copy link
Member

@rpanackal rpanackal commented Feb 26, 2025

Context

Prerequisite: SAP/cloud-sdk-java#734

AI/ai-sdk-java-backlog#203.

We are prepping for release. We also later plan to deliver a behaviour change of openai generator that captures array of numbers as float[] instead of List<BigDecimal>.

The generator will effectively also change the model class in a breaking manner. To avoid breaking changes in the future, we imitate the effect manually.

Feature scope:

  • Changes setters, getters and field
  • Changed equals, hashcode and toString (specifically noted for review)

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

@rpanackal rpanackal changed the title Chore/openai embedding float chore: [OpenAi] Embedding type change to float[] Feb 26, 2025
@rpanackal rpanackal changed the title chore: [OpenAi] Embedding type change to float[] chore: [OpenAI] Embedding type change to float[] Feb 26, 2025
@rpanackal rpanackal self-assigned this Feb 26, 2025
@rpanackal rpanackal added the please-review Request to review a pull-request label Feb 26, 2025
&& Objects.equals(this.index, embeddingsCreate200ResponseDataInner.index)
&& Objects.equals(this._object, embeddingsCreate200ResponseDataInner._object)
&& Objects.equals(this.embedding, embeddingsCreate200ResponseDataInner.embedding);
&& Arrays.equals(this.embedding, embeddingsCreate200ResponseDataInner.embedding);
Copy link
Member Author

@rpanackal rpanackal Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This equality check may continue with Objects.equals, especially if thats what the generator will do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Object.equals will be flagged a bug.

@Override
public int hashCode() {
return Objects.hash(index, _object, embedding, cloudSdkCustomFields);
return Objects.hash(index, _object, Arrays.hashCode(embedding), cloudSdkCustomFields);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, may continue without Arrays.hashCode()

Copy link
Contributor

@newtork newtork Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(How) do you plan to realize that in the generator mustache files?
Related SAP/cloud-sdk-java#734

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to take that as the case where there is no flag/indicator to distinguish between an array of primitive and object. So I will revert and stay consistent with all object type handling.

Copy link
Contributor

@newtork newtork Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly are you planning to do that? Are you currently working on a (the above) PR or is it a follow-up BLI?

Establishing manual code changes without a clear path to solve it in the generator is creating problems in the future. Same applies e.g. for prompt-registry PoC PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed and found a solution in Cloud SDK

@rpanackal rpanackal removed the please-review Request to review a pull-request label Feb 27, 2025
Copy link
Member Author

@rpanackal rpanackal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes required in equals and hashCode for field type float[] needs to be resolved as a minimum in the generator before this PR can be merged.

newtork
newtork previously approved these changes Feb 27, 2025
@rpanackal rpanackal merged commit fb2e716 into main Feb 27, 2025
6 checks passed
@rpanackal rpanackal deleted the chore/openai-embedding-float branch February 27, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants